Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the stream sending reliability #222

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

abel-von
Copy link
Contributor

Currently the send() method of stream implemented by send the value to an unbounded channel, so even the connection is closed for a long time, the send function still return succeed.

In this PR I add a channel to the message so that we can wait until the message is truely written to the connection.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 25.08%. Comparing base (a9198f2) to head (c09e90f).
Report is 11 commits behind head on master.

❗ Current head c09e90f differs from pull request most recent head 3355f7d. Consider uploading reports for the commit 3355f7d to get more accurate results

Files Patch % Lines
src/asynchronous/stream.rs 0.00% 27 Missing ⚠️
src/asynchronous/connection.rs 0.00% 6 Missing ⚠️
src/asynchronous/client.rs 0.00% 3 Missing ⚠️
src/asynchronous/server.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   24.97%   25.08%   +0.11%     
==========================================
  Files          16       16              
  Lines        2651     2719      +68     
==========================================
+ Hits          662      682      +20     
- Misses       1989     2037      +48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jsturtevant
Copy link
Collaborator

Is this something that where check could be added in https://github.com/containerd/ttrpc-rust/blob/master/example/async-client.rs? Similar to what was done in

// It verifies PR #208
async fn echo_default_value(

@jsturtevant
Copy link
Collaborator

looks like ci is failing with #219

Currently the send() method of stream implemented by send the value to
an unbounded channel, so even the connection is closed for a long time,
the send function still return succeed. This commit adds a channel to the
message so that we can wait until the message is truely written to the
connection.

Signed-off-by: Abel Feng <[email protected]>
@abel-von abel-von force-pushed the fix-reliability branch 2 times, most recently from c7fdd33 to 3355f7d Compare May 13, 2024 03:46
@abel-von
Copy link
Contributor Author

Is this something that where check could be added in https://github.com/containerd/ttrpc-rust/blob/master/example/async-client.rs? Similar to what was done in

// It verifies PR #208
async fn echo_default_value(

I think it is hard to write test here because it is some exceptional cases, for example server side restart. or we can do it by disconnecting the underlying connection, but it seems we have no way to do it in client directly.

@abel-von
Copy link
Contributor Author

looks like ci is failing with #219

I think it is now fixed.

@wllenyj
Copy link
Collaborator

wllenyj commented May 15, 2024

This would make send synchronous, and it feels like this behavior should be implemented by the upper level user.
Or make it optional?

@abel-von
Copy link
Contributor Author

abel-von commented May 27, 2024

I think maybe it is the best if ttrpc-rust behaves the same with https://github.com/containerd/ttrpc. The send in ttrpc golang seems synchronized as the channel is a blocking channel, as defined in https://github.com/containerd/ttrpc/blob/main/server.go#L338. so if underlying connection is closed, the send will also failed.

I think maybe we can not say this is "synchronized" as we don't really waiting for a acknowledgement from the other endpoint. we just make sure that the data is written into the connection.

@Burning1020
Copy link
Member

@wllenyj @jsturtevant Could you please take a look at these PRs?

Copy link
Collaborator

@wllenyj wllenyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

abel-von added a commit to kuasar-io/ttrpc-rust that referenced this pull request Jul 30, 2024
Copy link
Member

@Tim-Zhang Tim-Zhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @abel-von

@Tim-Zhang Tim-Zhang merged commit 745d2be into containerd:master Sep 24, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants